DATACMNS-1034 - Introduced API to easily register bidirectional, type-based converters. #209

odrotbohm · 2017-04-20T08:14:31Z

Introduced ConverterBuilder which exposes API to register Spring GenericConverters for the use with a store module's CustomConversions. This allows simple registration of such converters using Java 8 lambdas.

ConverterAware converters = ConverterBuilder
  .reading(String.class, Long.class, it -> Long.valueOf(it))
  .andWriting(it -> Object::toString);

The setup can also be done from the reading side which would just need the method invocations inverted. The resulting ConverterAware will expose the registered converters so that they can be easily handed to a CustomConversions instance. Partial creation of either reading or writing converters is possible, too with the returned instance then only exposing one of the two converters.

…va 8 lambdas. Introduced ConverterBuilder which exposes API to register Spring GenericConverters for the use with a store module's CustomConversions. This allows simple registration of such converters using Java 8 lambdas. ConverterAware converters = ConverterBuilder.reading(String.class, Long.class, it -> Long.valueOf(it)).andWriting(it -> Object::toString); The setup can also be done from the reading side which would just need the method invocations inverted. The resulting ConverterAware will expose the registered converters so that they can be easily handed to a CustomConversions instance. Partial creation of either reading or writing converters is possible, too with the returned instance then only exposing one of the two converters.

gregturn · 2017-04-21T15:29:07Z

readme.md

@@ -31,7 +31,7 @@ This README as well as the [reference documentation](http://docs.spring.io/sprin

 The main project [website](http://projects.spring.io/spring-data/) contains links to basic project information such as source code, JavaDocs, Issue tracking, etc.

-For more detailed questions, please refer to [spring-data on stackoverflow](http://stackoverflow.com/questions/tagged/spring-data). If you are new to Spring as well as to Spring Data, look for information about [Spring projects](https://spring.io/projects). 
+For more detailed questions, please refer to [spring-data on stackoverflow](http://stackoverflow.com/questions/tagged/spring-data). If you are new to Spring as well as to Spring Data, look for information about [Spring projects](https://spring.io/projects).


Not the place to suggest moving to asciidoctor?

gregturn · 2017-04-21T15:29:51Z

src/main/java/org/springframework/data/convert/ConverterBuilder.java

+	 * @return
+	 */
+	static <S, T> ReadingConverterBuilder<T, S> reading(Class<T> source, Class<S> target,
+			Function<? super T, ? extends S> function) {


Class<T> source looks backward (S is source, right?)

Agreed, I flipped those.

gregturn · 2017-04-21T15:30:58Z

src/main/java/org/springframework/data/convert/ConverterBuilder.java

+	 * @since 2.0
+	 */
+	interface ReadingConverterBuilder<T, S> extends ConverterBuilder, ReadingConverterAware {
+


Should this be <S, T>?

gregturn · 2017-04-21T15:31:28Z

src/main/java/org/springframework/data/convert/ConverterBuilder.java

+
+	/**
+	 * A {@link ConverterBuilder} ware of both a reading and writing converter.
+	 *


gregturn · 2017-04-21T15:33:44Z

src/main/java/org/springframework/data/convert/DefaultConverterBuilder.java

+	@Override
+	public Set<GenericConverter> getConverters() {
+		return Optionals.toStream(getOptionalReadingConverter(), getOptionalWritingConverter()).collect(Collectors.toSet());
+	}


Maybe it's just me, but I almost prefer reading this Java 8 stream as..

return Optionals .toStream(...) .collect(...)

Even if this results in an extra blank line to comply. ¯\_(ツ)_/¯

gregturn · 2017-04-21T15:36:18Z

src/main/java/org/springframework/data/convert/DefaultConverterBuilder.java

+		private static class Writing<S, T> extends ConfigurableGenericConverter<S, T> {
+
+			public Writing(ConvertiblePair convertiblePair, Function<? super S, ? extends T> function) {
+				super(convertiblePair, function);


Is public visibility modifier required?

gregturn · 2017-04-21T15:36:27Z

src/main/java/org/springframework/data/convert/DefaultConverterBuilder.java

+		private static class Reading<S, T> extends ConfigurableGenericConverter<S, T> {
+
+			public Reading(ConvertiblePair convertiblePair, Function<? super S, ? extends T> function) {
+				super(convertiblePair, function);


Public required?

gregturn · 2017-04-21T15:37:55Z

src/main/java/org/springframework/data/web/PagedResourcesAssembler.java

+
+	/**
+	 * Creates a {@link PagedResources} with an empt collection {@link EmbeddedWrapper} for the given domain type.
+	 * 


gregturn · 2017-04-21T15:38:27Z

src/main/java/org/springframework/data/web/PagedResourcesAssembler.java

 	}

 	/**
 	 * Creates a {@link PagedResources} with an empt collection {@link EmbeddedWrapper} for the given domain type.
 	 * 
 	 * @param page must not be {@literal null}, content must be empty.
 	 * @param type must not be {@literal null}.
-	 * @param link can be {@literal null}.
+	 * @param link must not be {@literal null}.
 	 * @return


Is this actually the place for specify this, given the assertion isn't here?

gregturn · 2017-04-21T15:43:47Z

src/test/java/org/springframework/data/convert/ConverterBuilderUnitTests.java

+	private static void assertOnlyConverter(ConverterBuilder builder, Supplier<GenericConverter> supplier) {
+		assertThat(builder.getConverters()).containsExactly(supplier.get());
+	}
+}


This test case is simply beautiful.

odrotbohm · 2017-04-21T15:47:58Z

Somehow some bogus commits slipped into this one. Removing.

Made nested API interfaces public so that they can actually be used. CustomConversions now considers ConverterAware and treats them appropriately for ConvertiblePair registration as well as during the registration in the ConversionService. Tweaked parameter types in CustomConversions to rather accept a Collection<?> for the converters (previously List<?>). Also, registerConverterIn(…) now takes a ConverterRegistry over a GenericConversionService. Polished Javadoc and non-null assertions.

odrotbohm · 2017-04-21T15:58:36Z

Tweaked code according to @gregturn's suggestions.

mp911de

Left some comments about minor issues and thoughts. LGTM, besides some typos.

mp911de · 2017-04-24T17:16:24Z

src/main/java/org/springframework/data/convert/ConverterBuilder.java

+	 * @author Oliver Gierke
+	 * @since 2.0
+	 */
+	public interface ConverterAware extends ConverterBuilder, ReadingConverterAware, WritingConverterAware {}


ConverterBuilder implies mutability whereas ConverterAware is the final result of the builder. It reads a bit strange.

Naming suggestions? ;)

mp911de · 2017-04-24T17:22:53Z

src/main/java/org/springframework/data/convert/ConverterBuilder.java

+	 * @author Oliver Gierke
+	 * @since 2.0
+	 */
+	public interface WritingConverterAware {


public not required. All nested types of in an interface inherit public visibility.

Interesting, I though at some point, Eclipse presented them as package protected so I thought the default of public only applied to methods. Rolling this back then.

mp911de · 2017-04-24T17:27:15Z

src/main/java/org/springframework/data/convert/CustomConversions.java

-				conversionService.addConverter(Converter.class.cast(it));
-				added = true;
-			}
+		boolean added = false;


I found it easier to read (understand) multiple return's instead of setting the added state:

if (candidate instanceof Converter) { conversionService.addConverter(Converter.class.cast(candidate)); return; }

so throw new IllegalArgumentException(…) would be the last and unconditional statement in the method body. Does this make sense?

At a quick glance, yes. Not at a second. The approach used allows the instance to implement multiple interfaces and thus obtains all ConvertibleType instances that implementing multiple ones would expose. An early return would stop that process after the first step.

Not sure this is still needed as I am unsure how likely this is in the first place. I just kept it around to preserve behavioural consistency to the old CustomConversions. We could think about removing that functionality, though.

mp911de · 2017-04-24T19:05:33Z

src/main/java/org/springframework/data/convert/ConverterBuilder.java

+
+/**
+ * API to easily set up {@link GenericConverter} instances using Java 8 lambdas, mostly in bidirectional fashion for
+ * easy registration as custom type converters of the SPring Data mapping subsystem. The registration starts either with


s/SPring/Spring

mp911de · 2017-04-24T19:06:00Z

src/main/java/org/springframework/data/convert/ConverterBuilder.java

+/**
+ * API to easily set up {@link GenericConverter} instances using Java 8 lambdas, mostly in bidirectional fashion for
+ * easy registration as custom type converters of the SPring Data mapping subsystem. The registration starts either with
+ * the definition of a reading or writing converter that can then be completed


Add dot at the end.

mp911de · 2017-04-24T19:06:54Z

src/main/java/org/springframework/data/convert/ConverterBuilder.java

+
+	/**
+	 * Creates a new {@link WritingConverterBuilder} to produce a converter to write values of the given source (the
+	 * domain type type) into the given target (the store type).


there's 2x type in domain type type

Typos in ConverterBuilder. Removed obsolete visibility modifiers for nested types in ConverterBuilder.

…va 8 lambdas. Introduced ConverterBuilder which exposes API to register Spring GenericConverters for the use with a store module's CustomConversions. This allows simple registration of such converters using Java 8 lambdas. ConverterAware converters = ConverterBuilder.reading(String.class, Long.class, it -> Long.valueOf(it)).andWriting(it -> Object::toString); The setup can also be done from the reading side which would just need the method invocations inverted. The resulting ConverterAware will expose the registered converters so that they can be easily handed to a CustomConversions instance. Partial creation of either reading or writing converters is possible, too with the returned instance then only exposing one of the two converters. CustomConversions now considers ConverterAware and treats them appropriately for ConvertiblePair registration as well as during the registration in the ConversionService. Tweaked parameter types in CustomConversions to rather accept a Collection<?> for the converters (previously List<?>). Also, registerConverterIn(…) now takes a ConverterRegistry over a GenericConversionService. Polished Javadoc and non-null assertions. Original pull request: #209.

odrotbohm requested review from schauder, gregturn, mp911de, christophstrobl and jxblum April 20, 2017 08:16

odrotbohm force-pushed the master branch from 9835c34 to 233fde3 Compare April 21, 2017 14:49

odrotbohm added 2 commits April 21, 2017 16:49

DATACMNS-1034 - Prepare feature branch.

dc14487

gregturn reviewed Apr 21, 2017

View reviewed changes

odrotbohm force-pushed the issue/DATACMNS-1034 branch from 3f73bc7 to 3436c63 Compare April 21, 2017 15:54

DATACMNS-1034 - Incorporate feedback from code review.

20d2f43

mp911de reviewed Apr 24, 2017

View reviewed changes

DATACMNS-1034 - Incorporate feedback from code review.

04a148a

Typos in ConverterBuilder. Removed obsolete visibility modifiers for nested types in ConverterBuilder.

odrotbohm closed this May 5, 2017

odrotbohm deleted the issue/DATACMNS-1034 branch May 5, 2017 12:55

spring-projects-issues mentioned this pull request Dec 30, 2020

Add API to easily register bidirectional, type-based converters [DATACMNS-1034] #1482

Closed

DATACMNS-1034 - Introduced API to easily register bidirectional, type-based converters. #209

DATACMNS-1034 - Introduced API to easily register bidirectional, type-based converters. #209

Uh oh!

Conversation

odrotbohm commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregturn Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odrotbohm commented Apr 21, 2017

Uh oh!

odrotbohm commented Apr 21, 2017

Uh oh!

mp911de left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

odrotbohm commented Apr 20, 2017 •

edited

Loading

gregturn Apr 21, 2017 •

edited

Loading